-
Notifications
You must be signed in to change notification settings - Fork 1.4k
refactor file to take better account of layers. #70
Conversation
Thanks for the contribution! However, these files are intended to be educational for those less familiar with Dockerfiles rather than being optimized for deployment, so we'll definitely want the comments to remain so it is clear as to what is happening. It's also important to have Finally, to speed up the process of iterating on the dev container, we should keep |
I have altered the front end statement, however, whilst I agree that the comments are useful, if they are intended for an audience that is not familiar with Docker, then perhaps also the layers should be optimal. People new to docker do not necessarily know this is an important thing that increases the size of the image. Perhaps there is a way to include the comments but also correct the way in which the file is structured. It is not just an optimisation but indeed it is listed as docker best practise on their website. And perhaps people newer to the technology are best served by following that best practice? |
It looks like we should be able to layer in the comments within the single RUN statement. This works for typescript-node-8: #-------------------------------------------------------------------------------------------------------------
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License. See https://go.microsoft.com/fwlink/?linkid=2090316 for license information.
#-------------------------------------------------------------------------------------------------------------
FROM node:8
ENV DEBIAN_FRONTEND=noninteractive
ENV SHELL /bin/bash
RUN apt-get update \
#
# Install apt-utils to avoid warnings
&& apt-get -y install --no-install-recommends apt-utils 2>&1 \
#
# Verify git and needed tools are installed
&& apt-get install -y \
git \
procps \
curl \
apt-transport-https \
lsb-release \
#
# Remove outdated yarn from /opt and install via package
# so it can be easily updated via apt-get upgrade yarn
&& rm -rf /opt/yarn-* \
&& rm -f /usr/local/bin/yarn \
&& rm -f /usr/local/bin/yarnpkg \
&& curl -sS https://dl.yarnpkg.com/$(lsb_release -is | tr '[:upper:]' '[:lower:]')/pubkey.gpg | apt-key add - 2>/dev/null \
&& echo "deb https://dl.yarnpkg.com/$(lsb_release -is | tr '[:upper:]' '[:lower:]')/ stable main" | tee /etc/apt/sources.list.d/yarn.list \
&& apt-get update \
&& apt-get -y install --no-install-recommends yarn \
#
# Install tslint and typescript globally
&& yarn global add \
tslint \
typescript \
#
# Clean up
&& apt-get autoremove -y \
&& apt-get clean -y \
&& rm -rf /var/lib/apt/lists/*
ENV DEBIAN_FRONTEND=dialog |
Great I will make the changes as described, it's all good |
changes have been made as suggested, inline comments placed back in, to enable people who are newer to docker to understand what is meant by certain operations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chuxel any thoughts about my points regarding this PR?
|
||
# Verify git and needed tools are installed | ||
RUN apt-get install -y git procps | ||
ENV DEBIAN_FRONTEND=dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not contribute to the purpose of using ENV DEBIAN_FRONTEND=noninteractive
as you are trying to reassign immediately with dialog
. We are using noninteractive because the following layers as per the original Dockerfile should run without user intervention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was not sure about this either
# Verify git and needed tools are installed | ||
RUN apt-get install -y git procps | ||
ENV DEBIAN_FRONTEND=dialog | ||
ENV SHELL /bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not seem a necessary optimization as we have been using a standard template for creating Dockerfiles for dev containers. You can find the standard template in the repository and it seems very easy, maintainable and clean in future using the standard template for all the dev containers 😄
RUN apt-get update \ | ||
&& apt-get -y install --no-install-recommends apt-utils 2>&1 \ | ||
&& apt-get install -y git procps \ | ||
&& rm -rf /opt/yarn-* \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging all the RUN
commands into a single layer will be a good optimization. But, As mentioned earlier we are using a template for Dockerfiles, which means few layers are in common among different dev containers so this means we are actually optimizing the docker build process by dividing them into different RUN
layers wherever necessary for better caching. So dev containers with similar layers will build faster due to cache but in your case and running all commands in a single intermediate container has least possibility to match other dev containers so making the build process slow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think as long as we can make it clear with comments and the "&&" being before the line so people can see that it is a continuation, we are probably okay here. The main concern is getting things so it is "cut-and-pasteable" into another definition. That was my main comment on that point here. It makes it clearer that the commands are a continuation of a broader command.
&& apt-get clean -y \ | ||
&& rm -rf /var/lib/apt/lists/* | ||
ENV DEBIAN_FRONTEND=dialog |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shifting this layer, as mentioned earlier does not contribute to the purpose.
@krystan So, I'm thinking that if we move forward with this, we'll probably want to do this largely across the board which fortunately isn't too hard -- but I want to get some regression testing scripts in place to make it a bit easier to retest things that are known to work. Looking thorugh @kmehant's comments, I think these are primarily are due to the fact that it looks like the JavaScript flavor didn't get updated like the TypeScript ones (since those already have the updates we discussed). |
ok is that something you need some help with or should I leave this for now ? |
So what I did was started a working branch for these: https://github.com/microsoft/vscode-dev-containers/tree/layer-consolidation What might be the best thing to do would be to actually do a PR into this branch instead so we can make these changes w/o impacting releases if needed. Mainly worrying about unexpected regressions due to small typos, etc. I did a pass but left the typescript / JS definitions alone so we can merge it in. That work? //cc: @chrmarti as FYI |
Made changes to the layout of the Dockerfile in typescript containers to make more efficient use of layers.